Skip to content

Conversation

@LevFendi
Copy link
Member

@LevFendi LevFendi commented Jan 2, 2026

@LevFendi LevFendi requested a review from ahicks92 January 2, 2026 14:18
@LevFendi LevFendi self-assigned this Jan 2, 2026
@LevFendi LevFendi added bug Something isn't working graphics About graphical stuff such as drawing functions, GUI's, and mouse pointer movement labels Jan 2, 2026
@LevFendi LevFendi changed the title Fendi 2 0 graphics Fendi 2.0 graphics fixes Jan 2, 2026
@LevFendi LevFendi marked this pull request as ready for review January 2, 2026 15:17
@EphDoering
Copy link

Since cursor pos is now always integral, it would be nice to cleanup the "special case" code which is actually always run now. I find it confusing to nudge the cursor pos to not be integral just so we can use ceiling and floor to get back the orginal and orignal plus one.

For the addition to adjust_cursor_size could that just be replaced with a single call to sync_build_cursor_graphics? If not, is there a differnt similar sync type function that could be called? Ideally any logic requiring knowing if you need to ceiling or floor or whatnot sould be contianed inside graphics.lua as all the code outside of it should be able to be tested and maintained without being able to visaully check the results.

@ahicks92
Copy link
Member

ahicks92 commented Jan 2, 2026

Given how small this is and that it is in code that I didn't remove/seriously break I would be willing to land it, but before I do this is exactly what I was talking about yesterday.

If we are fine with a 1 tick delay the real fix is:

  • make scripts/graphics for these.
  • make scripts/graphics/tick-handler.lua or something with an on_tick that calls out to the other handlers.
  • make scripts/graphics/build-preview.lua which:
    • grabs the viewpoint module and pulls the info
    • Grabs the cursor_stack of the player and pulls the info
    • grabs router.lua and asks if a UI is open
    • updates the preview every tick (or every n ticks) if the ui is closed and the thing in the player's hand has a preview (or if this is just supposed to be for cursors regardless of what's in hand, do that instead).

My problem is that even with a synchronization function this turns into "did you call it in the right places?" If the module is decoupled this is not an issue, one can be confident it works as long as as the core mod APIs remain stable and functional. Otherwise, it is tied to the specific flow of events. The cost is a 1 tick delay, which idk how significant that is in mp other than I was told that we can't have a 1 tick delay for speech once upon a time.

The small handler modules should be shorter than this comment tbh. It's hard-seeming when written out but shouldn't be after you do one.

To view it another way, if there is a sync function and it is always safe to call, then we just always call it every tick. If the sync function cannot be called every tick, then it is not in fact safe to call from anywhere, thus failing at decoupling. We can deal with performance once/if we find out it's too slow. But the model wherein we have to remember to call it in x places instead of 0 places is not scalable long-term, especially with blind maintainers, especially if Claude is maintaining the mod, and extra especially if those two are put together.

@EphDoering
Copy link

Yeah, I took a look at all calls to draw_large_cursor and I think they could all be turned into calls to a new function sync_large_cursor with only pindex as an argument. That would consolidate a few dozen duplicate code lines across control.lua and graphics.lua.

Oh just saw Austin's comment, and yeah that makes a lot of sense. Part of me is against adding things to do every tick, but if it's not a performance issue, it'll make the code a lot cleaner.

@ahicks92
Copy link
Member

ahicks92 commented Jan 2, 2026

We can do it every xth tick with a hinting function. There are enough ways to solve the performance problem that I would rather have the performance problem. You do:

function handler(pindex)
    if our_state[pindex].woke_up or this_tick%interval then do magic end
    our_state[pindex].woke_up = false
end

And the worst case of your bug becomes latency.

By picking a coprime set of intervals (which we do not need to do today) we can guarantee that the game rarely runs all of our tick handlers on the same frame, while additionally keeping the latency low. There is some allusion to someone realizing this in control.lua already, but I think that is probably more accidental picking of numbers and not actually someone realizing this property.

@ahicks92
Copy link
Member

ahicks92 commented Jan 2, 2026

Sorry to double post but to be clear, the open question is whether you want to land this or to do the more robust version.

I will accept this one but if you choose not to design a robust version in future we need to talk about what justifies graphics code kudzu. There's 3 tiers in my model, I am fine if it falls into 2 or 3 but fixes like this fall into 1:

  1. It's important enough that it's Austin's/everyone's problem (in a sense I am a proxy for Claude and other blind devs etc by virtue of being who's here, so don't overfixate on Austin)
  2. It's nice to have and it can be decoupled
  3. We don't need it

@LevFendi
Copy link
Member Author

LevFendi commented Jan 3, 2026

Calling "sync_build_cursor_graphics" instead in "adjust_cursor_size" did not work. On the other hand, turns out the flooring has no effect here, so I simplified the call by removing it.

As for special cases, overhauls, and wider discussions: The point of the PR was to precisely fix the existing graphics code so that we don't release with broken graphics. I don't think this PR is the place to talk about all that. Open the issue for "nuke all graphics code" or whatever and we can discuss it there and point others to it in the future.

@LevFendi
Copy link
Member Author

LevFendi commented Jan 3, 2026

There is some allusion to someone realizing this in control.lua already, but I think that is probably more accidental picking of numbers and not actually someone realizing this property.

Also let me let you know that intentional load distribution across different ticks was very much the goal there, even though that code has nothing to do with this PR.

@ahicks92
Copy link
Member

ahicks92 commented Jan 3, 2026

People land on the coprime solution either intentionally or unintentionally, that's not a dig or something. It is a slightly stronger solution than distributing the load because it guarantees maximum intervals between overlaps, even for otherwise small numbers. But mostly this point is about addressing Eph's concern.

I doubt graphics is fixed more generally. The reason I'm discussing things here is because I am expecting more near-term graphical fixes. I will merge momentarily. I will also say that on the other hand my solution should be very simple and obviously correct and not time consuming to implement, which is why some pushback from me here. Because we didn't discuss this first it is what it is, but if you are going to find a bunch of other bugs and fix them then please work with me on a design before you do that or we will just have to do all of this again sometime in the future when someone breaks it and doesn't notice.

@ahicks92 ahicks92 merged commit d541cc8 into main Jan 3, 2026
2 checks passed
@LevFendi LevFendi deleted the fendi-2-0-graphics branch January 3, 2026 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working graphics About graphical stuff such as drawing functions, GUI's, and mouse pointer movement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cursor box and mod cursor are mismatched

4 participants